Skip to content

Add method to retrieve currently configured strategies #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jun 14, 2020
Merged

Add method to retrieve currently configured strategies #172

merged 4 commits into from
Jun 14, 2020

Conversation

gsteel
Copy link
Contributor

@gsteel gsteel commented Jun 5, 2020 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets none
Documentation Will supply if appropriate/acceptable feature
License MIT

What's in this PR?

Adds a method to retrieve the currently configured discovery strategies.

Why?

When testing a consumer, it is useful to clear strategies to simulate a Not Found Exception, i.e. Psr18ClientDiscovery::setStrategies([])' but in order to reinstate the default behaviour of the lib after doing this, strategies would need to be duplicated in consumer tests.

Example Usage

$currentStrategies = Psr18ClientDiscovery::getStrategies();
Psr18ClientDiscovery::setStrategies([]);
try {
  // Test my thing…
  $this->fail('An exception was not thrown');
} catch (ExpectedError $error) {
  $this->makeMoreAssertions();
} finally {
  Psr18ClientDiscovery::setStrategies($currentStrategies);
}

Checklist

  • Updated CHANGELOG.md to describe new feature
  • Documentation pull request created (if not simply a bugfix)

To Do

  • Tests… Sorry, I'm not familiar with phpspec but would be happy to write a unit tests??

dbu
dbu approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems a valid reason to me. as we have the setter, having a way to know and restore the previous state seems useful to me.

for the test, maybe you can take this test as a starting point?
https://github.com/php-http/discovery/blob/master/spec/ClassDiscoverySpec.php#L61 (the main difference is that in phpspec, you call $this-> to call something on the class you are testing.) so in your case $this->getStrategies()->shouldReturn([...]);

can you please rebase on master? there is a conflict, i assume because of another change that also added to the changelog.

@gsteel
Copy link
Contributor Author

gsteel commented Jun 11, 2020

Thanks for the phpspec pointers @dbu - pull duly rebased. Sorry for the noisy commits!

dbu
dbu approved these changes Jun 14, 2020
View reviewed changes
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@dbu dbu merged commit 27903aa into php-http:master Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbu dbu dbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gsteel @dbu